fix(discussions): find closest discussion comment or reply by timestamp#2135
fix(discussions): find closest discussion comment or reply by timestamp#2135
Conversation
Signed-off-by: Adam Setch <adam.setch@outlook.com>
|
still need to update unit tests, but raising a draft pr for feedback from @afonsojramos @bmulholland |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where discussion notifications were showing incorrect actor profile images and deep-linking to the wrong discussion item. The fix changes the logic from finding the latest discussion comment/reply to finding the closest one by timestamp to the notification's updated_at time.
- Replaced simple "latest" selection logic with timestamp-based proximity matching
- Increased the number of fetched comments and replies from 1 to 100 for better accuracy
- Updated function names and signatures to reflect the new closest-match behavior
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/renderer/utils/subject.ts | Implements new closest-match algorithm using date-fns for timestamp comparison |
| src/renderer/utils/helpers.ts | Updates function call to use new closest-match logic |
| src/renderer/utils/api/client.ts | Increases fetched comment/reply limit from 1 to 100 for better matching accuracy |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| lastComments: 1, | ||
| lastReplies: 1, | ||
| lastComments: 100, | ||
| lastReplies: 100, |
There was a problem hiding this comment.
Increasing from 1 to 100 comments significantly increases the API response size and processing time. Consider using a smaller number like 10-20 initially, or make this configurable based on the actual need.
| lastReplies: 100, | |
| lastComments: options?.lastComments ?? 20, | |
| lastReplies: options?.lastReplies ?? 20, |
| lastComments: 1, | ||
| lastReplies: 1, | ||
| lastComments: 100, | ||
| lastReplies: 100, |
There was a problem hiding this comment.
Increasing from 1 to 100 replies significantly increases the API response size and processing time. Consider using a smaller number like 10-20 initially, or make this configurable based on the actual need.
| lastReplies: 100, | |
| lastReplies, |
Signed-off-by: Adam Setch <adam.setch@outlook.com>
…s-nearest Signed-off-by: Adam Setch <adam.setch@outlook.com>
…s-nearest Signed-off-by: Adam Setch <adam.setch@outlook.com>
|
Problem
Earlier today I noticed several discussion notifications which were showing an incorrect actor profile image, and upon notification interaction deep-linking to the wrong discussion item (latest comment instead of most recent reply on an older comment thread).
Solution
Updating our logic to find the latest discussion comment or reply to instead find the closest/nearest in timestamp.
In order to increase the accuracy, we have to fetch a wider selection of comments or replies before filtering down